Skip to content

server: fix root disk size on vm reset#4638

Merged
yadvr merged 2 commits intoapache:4.14from
shapeblue:fix-vm-rootsize-reset-3957
Apr 10, 2021
Merged

server: fix root disk size on vm reset#4638
yadvr merged 2 commits intoapache:4.14from
shapeblue:fix-vm-rootsize-reset-3957

Conversation

@shwstppr
Copy link
Contributor

@shwstppr shwstppr commented Feb 2, 2021

Description

If VM details contain rootdisksize, volume entry in DB should reflect correct size when VM reset is performed.

Fixes #3957

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

On resize of ROOT volume add detail in the VM for `rootdisksize` which will allow resizing root disk when VM reset is performed.

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
@shwstppr shwstppr linked an issue Feb 2, 2021 that may be closed by this pull request
@shwstppr shwstppr added this to the 4.14.1.0 milestone Feb 2, 2021
@yadvr yadvr modified the milestones: 4.14.1.0, 4.15.1.0 Feb 4, 2021
@DaanHoogland
Copy link
Contributor

code looks good @shwstppr but have you verified that the resize code is being called on a vm reset?

@shwstppr
Copy link
Contributor Author

@DaanHoogland I stopped work on this after your comment on the issue and reconsidering reset behaviour.
I think to support not reverting root disk size to the original value, maybe we can have a new param in resetVirtualMachine API. Default behaviour will revert size but user can call the API with the new flag to not revert it. However, that may require additional changes and testing.

@DaanHoogland
Copy link
Contributor

@shwstppr (cc @Spaceman1984 ) the consensus so far is to revert to the template including the template root size disk. making it optional is outside of this scope.

@yadvr
Copy link
Member

yadvr commented Feb 18, 2021

@Pearl1594
Copy link
Contributor

@blueorangutan package

1 similar comment
@Pearl1594
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✔️ centos7 ✔️ debian. SL-JID 271

@Pearl1594
Copy link
Contributor

@blueorangutan test centos7 vmware-67u3

@blueorangutan
Copy link

@Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests

@blueorangutan
Copy link

Trillian test result (tid-276)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 53529 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4638-t276-vmware-67u3.zip
Intermittent failure detected: /marvin/tests/smoke/test_diagnostics.py
Intermittent failure detected: /marvin/tests/smoke/test_network.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_iptables_default_policy.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_usage.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Smoke tests completed. 80 look OK, 3 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_05_ping_in_cpvm_success Failure 15.33 test_diagnostics.py
test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true Failure 319.79 test_routers_network_ops.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL Failure 605.11 test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers Failure 530.36 test_vpc_redundant.py
test_05_rvpc_multi_tiers Failure 558.01 test_vpc_redundant.py

@Pearl1594
Copy link
Contributor

@blueorangutan test centos7 vmware-67u3

@blueorangutan
Copy link

@Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests

@blueorangutan
Copy link

Trillian test result (tid-306)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 44095 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4638-t306-vmware-67u3.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Smoke tests completed. 82 look OK, 1 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_03_deploy_and_upgrade_kubernetes_cluster Error 3774.44 test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown Error 128.96 test_kubernetes_clusters.py

@shwstppr shwstppr marked this pull request as ready for review April 9, 2021 06:24
@shwstppr shwstppr requested a review from DaanHoogland April 9, 2021 06:25
Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, doing a new build/smoke test run on kvm

@blueorangutan
Copy link

Packaging result: ✔️ centos7 ✔️ debian. SL-JID 371

Copy link
Contributor

@Pearl1594 Pearl1594 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified behavior. LGTM

  1. Created a VM with a template of size 3GB but overrode root disk size to 8GB. Reinstalled the VM. The re-installed VM's root disk size = 8GB
  2. Created a VM with a template of size 3GB but overrode root disk size to 8GB. Performed a volume resize operation - volume size (root) = 15GB. Reinstalled VM - the reinstalled VM's root volume size = 15GB
  3. Created a VM with a template of size 3GB. Resized the volume of the VM to 8GB. Reinstalled the VM. Re-installed VM's root disk size = 3GB - DB updated as well

if (userVmDetailsDao.findDetail(vm.getId(), VmDetailConstants.ROOT_DISK_SIZE) == null && !newVol.getSize().equals(template.getSize())) {
VolumeVO resizedVolume = (VolumeVO) newVol;
resizedVolume.setSize(template.getSize());
_volsDao.update(resizedVolume.getId(), resizedVolume);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shwstppr it is possible to handle this case while allocating duplicate volume with template ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sureshanaparti Duplicating using template, won't that affect other existing values of the entry - folder, path, etc?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this comment, if we need to address anything additional pl raise a new PR @shwstppr @sureshanaparti


// 1. Save usage event and update resource count for user vm volumes
_resourceLimitMgr.incrementResourceCount(newVol.getAccountId(), ResourceType.volume, newVol.isDisplay());
_resourceLimitMgr.incrementResourceCount(newVol.getAccountId(), ResourceType.primary_storage, newVol.isDisplay(), new Long(newVol.getSize()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newVol here doesn't have updated template size, refresh it after the template size is updated in DB.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sureshanaparti same object is been cast to VolumeVO and size is updated at 6682-6683. newVol will return the updated size

@yadvr yadvr merged commit 6b1c94e into apache:4.14 Apr 10, 2021
@yadvr
Copy link
Member

yadvr commented Apr 10, 2021

Merged based on Daan, Pearl's review testing and smoketests

nlgordon pushed a commit to ippathways/cloudstack that referenced this pull request Aug 2, 2022
If VM details contain rootdisksize, volume entry in DB should reflect correct size when VM reset is performed.

Fixes apache#3957

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Co-authored-by: Pearl Dsilva <pearl.dsilva@shapeblue.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Root disk size reverts to its template size after rebuild

6 participants